-
-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
concretize methods #205
concretize methods #205
Conversation
Codecov Report
@@ Coverage Diff @@
## master #205 +/- ##
==========================================
- Coverage 71.12% 70.24% -0.89%
==========================================
Files 10 10
Lines 1562 1583 +21
==========================================
+ Hits 1111 1112 +1
- Misses 451 471 +20
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I think a good set of traits would be
both will be false for FunctionOp |
@ChrisRackauckas does this match what you need for ode.jl? |
Looks like it. |
@ChrisRackauckas take a look? |
@ChrisRackauckas can you take a look? The interface is described in the PR description |
Question: are EDITAnswer: it returns an aliased array julia> x = ones(4,4);
julia> y = convert(AbstractArray, x);
julia> y[:] .= 0;
julia> x
4×4 Matrix{Float64}:
0.0 0.0 0.0 0.0
0.0 0.0 0.0 0.0
0.0 0.0 0.0 0.0
0.0 0.0 0.0 0.0 |
@ChrisRackauckas ping |
Sorry I missed this one so much, I'm not sure how. What the reason for the preference of |
SciMLOperators are like matrices, and ScalarOperators are like numbers. So |
I see what you mean. May be hard to use in practice unless we make `concretize(L::Union{Number,AbstractArray}) = L |
AbstractMatrix, | ||
Factorization, | ||
|
||
# SciMLOperators | ||
AbstractSciMLOperator, | ||
} | ||
) = convert(AbstractMatrix, L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AbstractMatrix, | |
Factorization, | |
# SciMLOperators | |
AbstractSciMLOperator, | |
} | |
) = convert(AbstractMatrix, L) | |
AbstractArray, | |
Factorization, | |
# SciMLOperators | |
AbstractSciMLOperator, | |
} | |
) = convert(AbstractArray, L) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that be extended to array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because, an ND array cannot be used as an operator, i.e. you can't mul!
or *
it to another array.
julia> ones(4,4,4) * ones(4)
ERROR: MethodError: no method matching *(::Array{Float64, 3}, ::Vector{Float64})
So we strictly want to return AbstractMatrix
types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, in the package, I've defined convert(::Type{AbstractMatrix}, L::MyOperator)
methods for each operator type like
Base.convert(::Type{AbstractMatrix}, ii::IdentityOperator) = Diagonal(ones(Bool, ii.len))
So calling convert with AbstractArray would lead to errors:
julia> convert(AbstractArray, DiagonalOperator(ones(4))) [0/110]
ERROR: MethodError: Cannot `convert` an object of type
MatrixOperator{Float64, LinearAlgebra.Diagonal{Float64, Vector{Float64}}, SciMLOperators.FilterKwargs{typeof(SciMLOperators.DEFAULT_UPDATE_FUNC), Tuple{}}, SciMLOperators.FilterKwargs{typeof(SciMLOperators.DEFAULT_UPDATE_FUNC), Tuple{}}} to an object of type
AbstractArray
Well this works for now. |
add to docs? edit: #211 |
Thanks, missed that. |
fix #196
@ChrisRackauckas what is the expected heuristic here? Currently this only checks if we can convert to
AbstractMatrix
. Not if that's efficient or preferable.Does
isconcrete(L) == true
implies we cangetindex
/setindex!
? Do we want separate traitscan_setindex, can_getindex
?EDIT
We can simplify the workflow to just two functions
isconvertible
indicating ifL
can be converted (cheaply) to anAbstractMatrix
. This is true for lazy combinations of ScalarOp, MatrixOp, but not for TensorProdOp, InvertedOp. For FunctionOp, the behaviour can be set with kwargisconvertible
.concretize(L)
returns a concrete type forL
. It is a no-op whenL
is already concrete.workflow in downstream packages: